-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[INFRA] Add SonarCloud analysis on CI build #906
Conversation
ae1c2b0
to
3cde2a9
Compare
.github/workflows/build.yml
Outdated
@@ -49,12 +50,12 @@ jobs: | |||
- name: Install dependencies | |||
run: npm ci | |||
- name: Lint check | |||
run: npm run lint-check | |||
run: npm run lint-check -- -f json -o build/eslint-reporter.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ why is this needed? if the eslint check fails, the build fails so no other steps are triggered including sonar, so this seems useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This generates the Eslint report, and we need it for Sonar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you want to say.
If there is a error we don't analyze with Sonar, and if there is no error, sonar display anything. so it's useless.
Maybe, we should failed the job if the lint failed, only at the Sonar step end.
- name: Build Application | ||
run: npm run build | ||
- name: Test Application | ||
id: 'test_unit' | ||
run: npm run test:unit | ||
run: npm run test:unit ${{ matrix.os.coverage }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good way to configure the step according to the os! I am not this is the best name, for now, it is ok (I don't have any best proposal 😸)
sonar-project.properties
Outdated
|
||
# This is the name and version displayed in the SonarCloud UI. | ||
sonar.projectName=bpmn-visualization | ||
sonar.projectVersion=0.8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should automate this on version changes: we can do it in another PR, but as soon as possible, otherwise this will become a mess and I don't want we to have to manage this manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked myself the same.
We need to change the version on the package.json, because it's corresponding to the last tag. But, here, we are on delopment on the next.
Maybe on the tag workflow, we need to change the current version, after the tag.
Ok to do that on another PR 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern was about the version sync, but yes we can also address the version that stays with the tagged version.
Some projects (I didn't remember which one, but it was a big like jest) use x.y.z-post
version after release, we could do the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can manage it with #909
@@ -103,6 +103,7 @@ | |||
"jest-html-reporter": "^3.3.0", | |||
"jest-image-snapshot": "^4.2.0", | |||
"jest-puppeteer": "^4.4.0", | |||
"jest-sonar": "^0.2.11", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ do you know if this has perf impact on our local dev i.e if you have seen processing time increase when running tests?
I will do some checks on my side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ubuntu
Before jest-sonar integration
- Unit test: 22s
- E2e test: 2m 22s
After jest-sonar integration
- Unit test: 26s
- E2e test: 2m 26s
MacOS
Before jest-sonar integration
- Unit test: 28s
- E2e test: 3m 32s
After jest-sonar integration
- Unit test: 25s
- E2e test: 3m 22s
Windows
Before jest-sonar integration
- Unit test: 31s
- E2e test: 3m 32s
After jest-sonar integration
- Unit test: 38s
- E2e test: 3m 59s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the figures
It looks ok
8c0928d
to
853bdf2
Compare
00ea52a
to
ee873ba
Compare
@@ -14,6 +14,9 @@ | |||
<a href="https://github.com/process-analytics/bpmn-visualization-js/actions"> | |||
<img alt="Build" src="https://github.com/process-analytics/bpmn-visualization-js/workflows/Build/badge.svg"> | |||
</a> | |||
<a href="https://sonarcloud.io/dashboard?id=process-analytics_bpmn-visualization-js"> | |||
<img alt="Coverage" src="https://sonarcloud.io/api/project_badges/measure?project=process-analytics_bpmn-visualization-js&metric=coverage"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to fix the ignored tests on test:e2e:coverage, because the performance tests run :( |
@@ -62,11 +62,11 @@ | |||
"lint-check": "tsc --noEmit && eslint \"*/**/*.{js,ts,tsx}\" NOTICE --max-warnings 0", | |||
"test": "run-s test:unit test:e2e", | |||
"test:unit": "jest --runInBand --config=./test/unit/jest.config.js", | |||
"test:unit:coverage": "jest --runInBand --config=./test/unit/jest.config.js --coverage", | |||
"test:unit:watch": "jest --runInBand --config=./test/unit/jest.config.js --coverage --watchAll", | |||
"test:unit:coverage": "npm run test:unit -- --coverage", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ did you try run-s
instead of npm (which probably fork a new process). This applies to all scripts that have been chandeg to update another script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think to do that ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's adapted to what we want to do.
Here we just want to run the same configuration as for the unit test, but with one more paramter (for the coverage).
I tried with run-s
. It doesn't work. After the documentation reading of run-s
, it's more to run mutliple scripts (it's not what we do).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So let's keep it
"test:unit:coverage": "jest --runInBand --config=./test/unit/jest.config.js --coverage", | ||
"test:unit:watch": "jest --runInBand --config=./test/unit/jest.config.js --coverage --watchAll", | ||
"test:unit:coverage": "npm run test:unit -- --coverage", | ||
"test:unit:watch": "npm run test:unit:coverage -- --watchAll", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use this command (nor know why we created it) so I won't be impacted.
If @aibcmars is ok with this change, let's do it (check the doc if there is any mention to it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the coverage was already run as part of the 'watch', so there is no behaviour change.
coverageThreshold: { | ||
global: { | ||
branches: 80, | ||
functions: 80, | ||
lines: 80, | ||
statements: 80, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ I guess this disable the 'fail build' detection when we are under the threshold?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@aibcmars can you check #906 (comment) and decide if this is ok to you?
…r-project.properties
- Configurate tslint & eslint report on Sonar
… lint check failed
Co-authored-by: Thomas Bouffard <[email protected]>
3e80f0c
to
ec18173
Compare
Kudos, SonarCloud Quality Gate passed! |
Add configuration to run a SonarCloud analysis on master branch and pull requests
Closes #823